Automated Test: date-algorithm-enhanced #366
Closed
+111
−10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* fix date override for fixed round robin + time zone in date override * check if slot is within working hours of fixed hosts * add test for date override in different time zone * fix date overrides for not fixed hosts (round robin) * code clean up * fix added test * use the correct timezone of user for date overrides --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com>
📝 WalkthroughWalkthroughThis change introduces timezone-aware handling for date overrides and working hours in the scheduling system. The override computation logic is updated to use timezone-aware offset calculations, and availability checking now incorporates date-specific overrides and working hours with proper timezone alignment. A new optional Changes
Sequence DiagramsequenceDiagram
actor Client
participant API as API (slots.ts)
participant Processor as Override Processor
participant Validator as Availability Validator
participant Store as Schedule Store
Client->>API: Request schedule with dateOverrides,<br/>workingHours, timezone
API->>Processor: Process overrides with timezone context
Processor->>Processor: Compute organizerUtcOffset<br/>& inviteeUtcOffset
Processor->>Processor: Calculate offset difference<br/>& adjust startTime/endTime
Processor->>Validator: Pass adjusted overrides<br/>& working hours
Validator->>Validator: For each slot:<br/>Check if within override window<br/>or working hours
Validator->>Validator: Mark unavailable slots<br/>outside boundaries
Validator->>Store: Return validated<br/>availability slots
Store->>Client: Return timezone-aware<br/>schedule
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/trpc/server/routers/viewer/slots.ts`:
- Around line 114-116: The comparison using === between two Dayjs objects
(dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset,
"minutes")) is wrong; replace it with Dayjs value equality using .isSame(), e.g.
call .isSame(...) on one of the adjusted Dayjs instances (optionally with a
granularity like "minute") so the condition correctly detects identical moments;
update the expression where this check occurs to use .isSame instead of ===.
- Around line 141-145: The working-hours check incorrectly computes both start
and end from slotStartTime; update the end calculation to use the slot's end
time (e.g., slotEndTime.hour() * 60 + slotEndTime.minute() or
slotStartTime.plus(duration) if slotEndTime variable is named differently) so
that the start/end values accurately reflect the slot span, then keep the
existing boundary check against workingHour.startTime and workingHour.endTime
using the corrected end variable; adjust any variable names in the block
containing start, end, slotStartTime and workingHour accordingly.
- Around line 138-151: The callback passed to workingHours.find currently only
returns true when a violation is detected and otherwise returns undefined;
update the callback for workingHours.find (the anonymous arrow function
referencing workingHour and slotStartTime) to always return a boolean — return
true when the slot is outside the workingHour bounds and explicitly return false
otherwise (and also ensure you compute start/end from the correct slot time
variables if needed) so find() behaves predictably.
- Around line 106-131: The find() callback for dateOverrides may return
undefined on some paths; change the logic so the predicate always returns a
boolean and use a clearer API (e.g., replace dateOverrides.find(...) with
dateOverrides.some(...)) and return explicit true/false from the callback.
Locate the block referencing dateOverrides.find, organizerTimeZone,
slotStartTime, slotEndTime and dateOverrideExist; set dateOverrideExist when a
matching date is found and have the callback return true for both "slot outside
override" and "slot equals/overlaps" cases and false otherwise (or use some() to
directly assign dateOverrideExist = dateOverrides.some(...)). Ensure no path
leaves the callback without an explicit boolean return.
Comment on lines
+106
to
+131
| dateOverrides.find((date) => { | ||
| const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0; | ||
|
|
||
| if ( | ||
| dayjs(date.start).add(utcOffset, "minutes").format("YYYY MM DD") === | ||
| slotStartTime.format("YYYY MM DD") | ||
| ) { | ||
| dateOverrideExist = true; | ||
| if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) { | ||
| return true; | ||
| } | ||
| if ( | ||
| slotEndTime.isBefore(dayjs(date.start).add(utcOffset, "minutes")) || | ||
| slotEndTime.isSame(dayjs(date.start).add(utcOffset, "minutes")) | ||
| ) { | ||
| return true; | ||
| } | ||
| if (slotStartTime.isAfter(dayjs(date.end).add(utcOffset, "minutes"))) { | ||
| return true; | ||
| } | ||
| } | ||
| }) | ||
| ) { | ||
| // slot is not within the date override | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: find() callback should always return a boolean value.
Static analysis correctly flags that this callback doesn't return a value on all paths. When the date matches but the slot is within the override bounds, undefined is implicitly returned. While this works due to JavaScript's truthy/falsy handling, it's error-prone and hard to reason about.
Consider refactoring to use some() with explicit returns, or restructure the logic:
♻️ Suggested refactor for clarity
- if (
- dateOverrides.find((date) => {
- const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0;
-
- if (
- dayjs(date.start).add(utcOffset, "minutes").format("YYYY MM DD") ===
- slotStartTime.format("YYYY MM DD")
- ) {
- dateOverrideExist = true;
- if (dayjs(date.start).add(utcOffset, "minutes").isSame(dayjs(date.end).add(utcOffset, "minutes"))) {
- return true;
- }
- if (
- slotEndTime.isBefore(dayjs(date.start).add(utcOffset, "minutes")) ||
- slotEndTime.isSame(dayjs(date.start).add(utcOffset, "minutes"))
- ) {
- return true;
- }
- if (slotStartTime.isAfter(dayjs(date.end).add(utcOffset, "minutes"))) {
- return true;
- }
- }
- })
- ) {
+ const slotOutsideOverride = dateOverrides.some((date) => {
+ const utcOffset = organizerTimeZone ? dayjs.tz(date.start, organizerTimeZone).utcOffset() * -1 : 0;
+ const overrideStart = dayjs(date.start).add(utcOffset, "minutes");
+ const overrideEnd = dayjs(date.end).add(utcOffset, "minutes");
+
+ if (overrideStart.format("YYYY MM DD") !== slotStartTime.format("YYYY MM DD")) {
+ return false; // Not the same day
+ }
+
+ dateOverrideExist = true;
+
+ // Check if slot is outside the override window
+ if (overrideStart.isSame(overrideEnd)) {
+ return true; // Zero-length override blocks the day
+ }
+ if (slotEndTime.isBefore(overrideStart) || slotEndTime.isSame(overrideStart)) {
+ return true;
+ }
+ if (slotStartTime.isAfter(overrideEnd)) {
+ return true;
+ }
+ return false; // Slot is within override
+ });
+
+ if (slotOutsideOverride) {
// slot is not within the date override
return false;
}🧰 Tools
🪛 Biome (2.3.13)
[error] 106-106: This callback passed to find() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/slots.ts` around lines 106 - 131, The
find() callback for dateOverrides may return undefined on some paths; change the
logic so the predicate always returns a boolean and use a clearer API (e.g.,
replace dateOverrides.find(...) with dateOverrides.some(...)) and return
explicit true/false from the callback. Locate the block referencing
dateOverrides.find, organizerTimeZone, slotStartTime, slotEndTime and
dateOverrideExist; set dateOverrideExist when a matching date is found and have
the callback return true for both "slot outside override" and "slot
equals/overlaps" cases and false otherwise (or use some() to directly assign
dateOverrideExist = dateOverrides.some(...)). Ensure no path leaves the callback
without an explicit boolean return.
Comment on lines
+114
to
+116
| if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Bug: Dayjs object comparison with === always returns false.
Comparing two Dayjs objects with === checks reference equality, not value equality. This condition will never be true, even when the start and end times are identical.
🐛 Proposed fix using `.isSame()`
- if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) {
+ if (dayjs(date.start).add(utcOffset, "minutes").isSame(dayjs(date.end).add(utcOffset, "minutes"))) {
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (dayjs(date.start).add(utcOffset, "minutes") === dayjs(date.end).add(utcOffset, "minutes")) { | |
| return true; | |
| } | |
| if (dayjs(date.start).add(utcOffset, "minutes").isSame(dayjs(date.end).add(utcOffset, "minutes"))) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/slots.ts` around lines 114 - 116, The
comparison using === between two Dayjs objects (dayjs(date.start).add(utcOffset,
"minutes") === dayjs(date.end).add(utcOffset, "minutes")) is wrong; replace it
with Dayjs value equality using .isSame(), e.g. call .isSame(...) on one of the
adjusted Dayjs instances (optionally with a granularity like "minute") so the
condition correctly detects identical moments; update the expression where this
check occurs to use .isSame instead of ===.
Comment on lines
+138
to
+151
| if ( | ||
| workingHours.find((workingHour) => { | ||
| if (workingHour.days.includes(slotStartTime.day())) { | ||
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| if (start < workingHour.startTime || end > workingHour.endTime) { | ||
| return true; | ||
| } | ||
| } | ||
| }) | ||
| ) { | ||
| // slot is outside of working hours | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Working hours find() callback should explicitly return a boolean.
Same issue as above - this callback only returns true when the slot is outside working hours, but implicitly returns undefined otherwise.
♻️ Suggested refactor
- if (
- workingHours.find((workingHour) => {
- if (workingHour.days.includes(slotStartTime.day())) {
- const start = slotStartTime.hour() * 60 + slotStartTime.minute();
- const end = slotEndTime.hour() * 60 + slotEndTime.minute();
- if (start < workingHour.startTime || end > workingHour.endTime) {
- return true;
- }
- }
- })
- ) {
+ const slotOutsideWorkingHours = workingHours.some((workingHour) => {
+ if (!workingHour.days.includes(slotStartTime.day())) {
+ return false;
+ }
+ const start = slotStartTime.hour() * 60 + slotStartTime.minute();
+ const end = slotEndTime.hour() * 60 + slotEndTime.minute();
+ return start < workingHour.startTime || end > workingHour.endTime;
+ });
+
+ if (slotOutsideWorkingHours) {
// slot is outside of working hours
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( | |
| workingHours.find((workingHour) => { | |
| if (workingHour.days.includes(slotStartTime.day())) { | |
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| if (start < workingHour.startTime || end > workingHour.endTime) { | |
| return true; | |
| } | |
| } | |
| }) | |
| ) { | |
| // slot is outside of working hours | |
| return false; | |
| } | |
| const slotOutsideWorkingHours = workingHours.some((workingHour) => { | |
| if (!workingHour.days.includes(slotStartTime.day())) { | |
| return false; | |
| } | |
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| const end = slotEndTime.hour() * 60 + slotEndTime.minute(); | |
| return start < workingHour.startTime || end > workingHour.endTime; | |
| }); | |
| if (slotOutsideWorkingHours) { | |
| // slot is outside of working hours | |
| return false; | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 139-139: This callback passed to find() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/slots.ts` around lines 138 - 151, The
callback passed to workingHours.find currently only returns true when a
violation is detected and otherwise returns undefined; update the callback for
workingHours.find (the anonymous arrow function referencing workingHour and
slotStartTime) to always return a boolean — return true when the slot is outside
the workingHour bounds and explicitly return false otherwise (and also ensure
you compute start/end from the correct slot time variables if needed) so find()
behaves predictably.
Comment on lines
+141
to
+145
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | ||
| if (start < workingHour.startTime || end > workingHour.endTime) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Critical: Both start and end are computed from slotStartTime, causing incorrect working hours validation.
Line 142 computes end identically to start, using slotStartTime for both. This means the working hours boundary check on line 143 is comparing start < workingHour.startTime || start > workingHour.endTime which doesn't account for the slot's duration.
🐛 Proposed fix
if (workingHour.days.includes(slotStartTime.day())) {
const start = slotStartTime.hour() * 60 + slotStartTime.minute();
- const end = slotStartTime.hour() * 60 + slotStartTime.minute();
+ const end = slotEndTime.hour() * 60 + slotEndTime.minute();
if (start < workingHour.startTime || end > workingHour.endTime) {
return true;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| const end = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| if (start < workingHour.startTime || end > workingHour.endTime) { | |
| return true; | |
| } | |
| const start = slotStartTime.hour() * 60 + slotStartTime.minute(); | |
| const end = slotEndTime.hour() * 60 + slotEndTime.minute(); | |
| if (start < workingHour.startTime || end > workingHour.endTime) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/slots.ts` around lines 141 - 145, The
working-hours check incorrectly computes both start and end from slotStartTime;
update the end calculation to use the slot's end time (e.g., slotEndTime.hour()
* 60 + slotEndTime.minute() or slotStartTime.plus(duration) if slotEndTime
variable is named differently) so that the start/end values accurately reflect
the slot span, then keep the existing boundary check against
workingHour.startTime and workingHour.endTime using the corrected end variable;
adjust any variable names in the block containing start, end, slotStartTime and
workingHour accordingly.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Tests
Bug Fixes